Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

examples/skills/server.py (1)

23-46: Make optional provider examples copy‑paste ready.
Uncommenting Options 1/3 currently requires manual imports; a short note avoids NameError surprises.

💡 Optional tweak

-# Option 1: Load a single skill
-# mcp.add_provider(SkillProvider(Path.home() / ".claude/skills/pdf-processing"))
+# Option 1: Load a single skill (add SkillProvider to the imports if you uncomment)
+# mcp.add_provider(SkillProvider(Path.home() / ".claude/skills/pdf-processing"))

-# Option 3: Load skills from a platform's default location
-# mcp.add_provider(ClaudeSkillsProvider())  # ~/.claude/skills/
+# Option 3: Load skills from a platform's default location
+# (add ClaudeSkillsProvider to the imports if you uncomment)
+# mcp.add_provider(ClaudeSkillsProvider())  # ~/.claude/skills/

docs/servers/providers/skills.mdx (1)

20-48: Convert Quick Start into a Steps-based, runnable procedure.

This section reads as a procedure but isn’t structured with a <Steps> component and doesn’t show a fully runnable flow with expected outcomes/error handling. Consider converting it to explicit steps and including a minimal runnable example (with basic error handling and expected output). As per coding guidelines, keep procedures in Steps and include runnable examples with outcomes.

src/fastmcp/server/providers/skills/_common.py (1)

86-99: Make manifest ordering deterministic.

rglob() order can vary by filesystem, which can lead to unstable manifests and tests. Consider sorting by relative path before building the list.

Proposed fix

-    for file_path in skill_dir.rglob("*"):
+    for file_path in sorted(skill_dir.rglob("*")):
         if file_path.is_file():
             rel_path = file_path.relative_to(skill_dir)
             files.append(
                 SkillFileInfo(
                     path=str(rel_path),
                     size=file_path.stat().st_size,
                     hash=compute_file_hash(file_path),
                 )
             )

src/fastmcp/server/providers/skills/directory_provider.py (1)

105-114: Consider catching broader exceptions during provider creation.

SkillProvider.__init__ can raise exceptions beyond FileNotFoundError (e.g., PermissionError, OSError for I/O issues, or exceptions from parse_frontmatter). Currently only FileNotFoundError is caught, which could cause a single problematic skill to prevent loading all subsequent skills in the loop.

♻️ Suggested improvement

                 try:
                     provider = SkillProvider(
                         skill_path=skill_dir,
                         main_file_name=self._main_file_name,
                         supporting_files=self._supporting_files,
                     )
                     self.providers.append(provider)
                     seen_skill_names.add(skill_name)
-                except FileNotFoundError:
+                except (FileNotFoundError, PermissionError, OSError) as e:
                     logger.exception(f"Failed to load skill: {skill_dir.name}")

src/fastmcp/server/providers/skills/skill_provider.py (2)

68-80: Path traversal protection is correctly implemented.

The security checks using resolve() and is_relative_to() effectively prevent directory traversal attacks. However, the exception handling can be simplified since the ValueError from is_relative_to() is already meaningful.

♻️ Optional simplification

         # Security: ensure path doesn't escape skill directory
-        try:
-            full_path = full_path.resolve()
-            if not full_path.is_relative_to(self.skill_info.path):
-                raise ValueError(f"Path {file_path} escapes skill directory")
-        except ValueError as e:
-            raise ValueError(f"Invalid path: {e}") from e
+        full_path = full_path.resolve()
+        if not full_path.is_relative_to(self.skill_info.path):
+            raise ValueError(f"Path {file_path} escapes skill directory")

---

131-147: Consider extracting shared file-reading logic.

The path validation and content reading logic is duplicated between SkillFileTemplate.read() (lines 68-87) and SkillFileResource.read() (lines 133-147). A shared helper method could reduce duplication.

src/fastmcp/server/providers/skills/vendor_providers.py (1)

29-44: VSCodeSkillsProvider and CopilotSkillsProvider share identical root path.

Both providers point to ~/.copilot/skills/ (lines 37 and 117). If both are added to a server, they'll expose duplicate resources for the same directory.

Either use a distinct path for VS Code (e.g., ~/.vscode/skills/) or document explicitly that this is intentional (VS Code shares GitHub Copilot's skills directory).

Also applies to: 109-124

src/fastmcp/server/providers/skills/_common.py (1)

33-74: Custom YAML parser has limited coverage — acceptable for simple frontmatter but may fail on edge cases.

The parser handles single-level key-value pairs, quoted strings, and inline arrays, which is sufficient for typical frontmatter. However, it doesn't support:

• Multi-line values
• Nested objects
• Block arrays (YAML list syntax with -)
• Boolean/numeric type coercion

This is acceptable given the comment at line 53 noting "simple key: value parsing, no complex types." Consider documenting these limitations in the docstring or falling back to a YAML library if more complex frontmatter is expected.

src/fastmcp/server/providers/skills/skill_provider.py (2)

38-44: Synchronous file I/O in async method may block the event loop.

main_file_path.read_text() at line 44 performs synchronous file I/O within an async method. For small skill files this is likely acceptable, but for larger files or high-concurrency scenarios, this could block the event loop.

Consider wrapping in asyncio.to_thread() if performance becomes a concern, though this is optional for the typical use case.

---

307-358: version parameter is accepted but unused.

The version parameter in _get_resource is never used, which the static analysis correctly flags (ARG002). Since versioning may be relevant for future resource management, consider either:

1. Passing it to the base class method if applicable
2. Documenting that versioning is not supported for skill resources

This is minor since the base Provider._get_resource signature requires this parameter.

src/fastmcp/server/providers/skills/directory_provider.py (1)

76-120: Synchronous discovery called from async context may block event loop.

_discover_skills() performs synchronous filesystem operations (root.exists(), root.iterdir(), main_file.exists()) and is called from async methods via _ensure_discovered(). With reload=True, this runs on every request and could block the event loop.

For directories with many skills, consider running discovery in a thread:

async def _ensure_discovered(self) -> None:
    if self._reload or not self._discovered:
        await asyncio.to_thread(self._discover_skills)

This is a recommended improvement for production use with reload enabled.

src/fastmcp/server/providers/skills/__init__.py (1)

43-44: Clarify the purpose of the "backwards compatibility" alias.

Since this is a new module being introduced in this PR, the "backwards compatibility alias" comment may be misleading. If SkillsProvider was used during development or in early documentation, consider adding a brief note explaining when/why this alias exists. Otherwise, if this is purely for API convenience (offering both singular and plural naming), consider updating the comment to reflect that intent.

📝 Suggested comment clarification

-# Backwards compatibility alias
+# Convenience alias: SkillsProvider is an alias for SkillsDirectoryProvider
 SkillsProvider = SkillsDirectoryProvider